Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add transaction items to log and trace requests #224

Merged
merged 3 commits into from
Jan 18, 2024

Conversation

belopash
Copy link
Contributor

No description provided.

@@ -140,6 +145,16 @@ export function filterBlock(block: Block, dataRequest: DataRequest): void {
if (rel.transaction) {
include.addTransaction(log.transaction)
}
if (rel.transactionLogs && log.transaction) {
Copy link
Contributor Author

@belopash belopash Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

archive response also doesn't include transaction if it is not explicitly requested with log.transaction

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.transactionLogs and .transactionTraces don't imply transaction presence. Hence, log inclusion should not rely on .transaction property.

groupBy(logs, log => log.transactionIndex) should be used instead.

@belopash belopash requested a review from eldargab January 15, 2024 12:21
@@ -140,6 +145,16 @@ export function filterBlock(block: Block, dataRequest: DataRequest): void {
if (rel.transaction) {
include.addTransaction(log.transaction)
}
if (rel.transactionLogs && log.transaction) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.transactionLogs and .transactionTraces don't imply transaction presence. Hence, log inclusion should not rely on .transaction property.

groupBy(logs, log => log.transactionIndex) should be used instead.

}
}
if (rel.transactionTraces && log.transaction) {
for (let trace of log.transaction.traces) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before, except this time presence of logs should be enforced here as well.

@@ -182,6 +197,11 @@ export function filterBlock(block: Block, dataRequest: DataRequest): void {
if (rel.transaction) {
include.addTransaction(trace.transaction)
}
if (rel.transactionLogs && trace.transaction) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@belopash belopash force-pushed the log-request-transaction-items branch from 4499d92 to 3719dd3 Compare January 17, 2024 09:21
@eldargab eldargab merged commit b65bc54 into master Jan 18, 2024
1 check passed
@eldargab eldargab deleted the log-request-transaction-items branch January 18, 2024 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants